Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix dynamic JS import and add missing scripts for colormaps #193

Merged
merged 4 commits into from
Jun 18, 2024

Conversation

Navxihziq
Copy link
Contributor

@Navxihziq Navxihziq commented Jun 16, 2024

This PR addresses several issues related to dynamic JavaScript packages imports and the not displayed colormap.

  1. JavaScript package loading order
    The script loading mechanism in index.tsx has been modified to ensure that JavaScript packages are imported sequentially. This should fix the issues with the vanishing overlay and feature groups caused by dependencies not being loaded in the correct order.

  2. Colormap display
    The d3 package, required for displaying the colormap, has been manually included. The previous dynamic JS and CSS dependency detection in __init__.py did not work for branca.colormap.ColorMap objects, leading to the colormap not being displayed.

With these changes, the dynamic package loading and colormap display issues should be resolved.

@Navxihziq Navxihziq closed this Jun 16, 2024
@Navxihziq Navxihziq reopened this Jun 16, 2024
@Navxihziq
Copy link
Contributor Author

The new commit adds the view change detection section to the index.tsx file, which was unintentionally omitted in the previous commit. The overall functionalities, including multiple feature groups loading and customized colormap legend displaying have been (momentarily) tested!

@randyzwitch
Copy link
Owner

@blackary The way I hear these issues people are having, it sounds like a race condition to me. For a common dependency like d3, I wonder if we should just add it to the JS component side of the house. Though, I'm not exactly certain the browser behavior if you have a dependency specified, then specify it again dynamically like this package now does

@randyzwitch randyzwitch requested a review from blackary June 17, 2024 13:47
@blackary
Copy link
Collaborator

@Navxihziq Currently the tests are failing from some small type-hint and formatting issues, you please use pre-commit to fix the linting issues, or fix them manually, or allow admins to commit to your branch?

@Navxihziq
Copy link
Contributor Author

Navxihziq commented Jun 18, 2024

@blackary Fixed!

@@ -5,10 +5,11 @@
import re
import warnings
from textwrap import dedent
from typing import Iterable
from typing import Iterable, List
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: Because of line 1, you can just use list[str] instead of importing List

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update committed!

Copy link
Collaborator

@blackary blackary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, and all the tests pass 👍

@randyzwitch randyzwitch merged commit 77c692c into randyzwitch:master Jun 18, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants